Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a minification issue with the __hasRegisterFinished property. #5584

Merged
merged 2 commits into from
Aug 27, 2019
Merged

Fix a minification issue with the __hasRegisterFinished property. #5584

merged 2 commits into from
Aug 27, 2019

Conversation

bicknellr
Copy link
Member

Wrap hasOwnProperty checks for __hasRegisterFinished in JSCompiler_renameProperty().

kevinpschaaf
kevinpschaaf requested changes Aug 26, 2019
View reviewed changes
Copy link
Member

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, I just searched the code and there are a few other hasOwnProperty checks with string literals that should be wrapped in JSCompiler_renameProperty:

  • properties-changed.js: __dataHasAccessor
  • properties-changed.js: __dataAttributes
  • properties-mixin.js: __observedAttributes

I spot checked these in compiled code and I don't see them being renamed (not entirely sure why), but for safety, let's go ahead and close the hole.

@bicknellr
Copy link
Member Author

I've never actually used JSCompiler_renameProperty and I wasn't able to find any documentation about the two-argument form, but from other uses I gathered that the second argument was meant to be the thing that you're going to pull the property from (i.e. thing[JSCompiler_renameProperty('propName', thing)]). Is that correct?

kevinpschaaf
kevinpschaaf approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's right, 2nd arg is the context object for the property being dereferenced.

LGTM

@kevinpschaaf kevinpschaaf merged commit f6ccc9d into master Aug 27, 2019
@kevinpschaaf kevinpschaaf deleted the closure-__hasRegisterFinished branch August 27, 2019 18:41
This was referenced Sep 7, 2024
Open
Open
This was referenced Sep 14, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinpschaaf kevinpschaaf kevinpschaaf approved these changes

@dfreedm dfreedm Awaiting requested review from dfreedm

@sorvell sorvell Awaiting requested review from sorvell

@TimvdLippe TimvdLippe Awaiting requested review from TimvdLippe

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bicknellr @kevinpschaaf @googlebot